Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ADR034: Account Rekeying #7491

Merged
merged 20 commits into from
Nov 27, 2020
Merged

Conversation

sunnya97
Copy link
Member

@sunnya97 sunnya97 commented Oct 9, 2020

Description

This PR includes an ADR for a new MsgType to allow the updating of the public key for an account.

Corresponding implementation in #7452


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@sunnya97 sunnya97 added T: ADR An issue or PR relating to an architectural decision record C:x/auth labels Oct 9, 2020
@clevinson
Copy link
Contributor

Thanks @sunnya97!

Can you update the # of this to ADR034 ? We currently have WIP ADR's up to ADR033 already as active pull requests.

@sunnya97 sunnya97 mentioned this pull request Oct 9, 2020
9 tasks

Breaks the current assumed relationship between address and pubkeys as H(pubkey) = address. This has a couple of consequences.

* We cannot prune accounts with 0 balance that have had their pubkey changed (we currently do not currently do this anyways, but the reason we have account numbers is presumably for this purpose).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can still make it possible to prune accounts by introducing a new message: MsgNukeAccount. When an author will delete HIS account, then the deposit is released.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I implemented the "deposit" as an extra Gas charge rather than an actual monetary deposit. With a gas charge, we could provide a gasrefund when an account is nuked, but that could potentially turn into GasToken shenanigans like on Ethereum.

@robert-zaremba
Copy link
Collaborator

I really like this ADR.

I also started drafting a similar thing 2 weeks ago: to decouple account address from a public key and proposed it as one of the goals for SDK. I call it re-keying (this is in fact a term used by Algorand). During the review of #7086 I noted we need to support accounts with native addresses, specifically:

  • multisigs with native public key
  • accounts which will enable dynamic keys.

@aaronc in #7086 (comment) pointed to a Group Module we are designing at Regen Network, which solves the latter point using a separation of an account object from the policies.

What's good, all this ideas will play together very well:

  • we can use the Address algorithm from ADR-028 to compute an initial address.
  • we can update all account structures to add a fixed address field, which will be computed using the aforementioned Address function.
  • All accounts will support re-keying.

@clevinson clevinson changed the title Change Pubkey ADR ADR034: Change Pubkey Oct 14, 2020
Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

Can you add a boolean EnableChangePubKey param?

docs/architecture/adr-034-change-pubkey.md Outdated Show resolved Hide resolved
docs/architecture/adr-034-change-pubkey.md Outdated Show resolved Hide resolved
docs/architecture/adr-034-change-pubkey.md Outdated Show resolved Hide resolved
docs/architecture/adr-034-change-pubkey.md Outdated Show resolved Hide resolved
@aaronc aaronc requested a review from blushi October 21, 2020 17:43
Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

One question: do we allow two accounts with the same pubkey but different addresses?

docs/architecture/adr-034-change-pubkey.md Outdated Show resolved Hide resolved
docs/architecture/adr-034-change-pubkey.md Outdated Show resolved Hide resolved
@zmanian
Copy link
Member

zmanian commented Oct 24, 2020

I'm mostly comfortable with this ADR.

here is my reasoning.

  1. It's presumably not a mandatory for a chain to implement this message type.

  2. I'd oppose implementing it on the hub without other fee improvements like consensus min fee but all of those things are coming.

@sunnya97
Copy link
Member Author

@aaronc Yeah, I think that should be allowed. I can't think of a reason it shouldn't be, as msgs explicitly declare the originating address, and don't depend on the signature/pubkey for that

docs/architecture/adr-034-change-pubkey.md Outdated Show resolved Hide resolved
docs/architecture/adr-034-change-pubkey.md Outdated Show resolved Hide resolved
docs/architecture/adr-034-change-pubkey.md Outdated Show resolved Hide resolved
@ethanfrey
Copy link
Contributor

Robert, I don't say Ethereum is perfect, and the points you raise have little to do with key rotation.

What I am saying is this separation of concepts has allowed people to reason about behavior - especially on the off-chain / wallet / layer 2 side. I do not see any clear analysis of how one could build off-chain infrastructure with this key rotations.

e.g. if I open a state channel with an account, I would only open it with a pubkey tied to an EOA. I cannot open it with some on-chain multisig contract as one party. So, I design a state channel solution with that understanding. Suddenly, even these stable addresses may switch out pubkeys while packets are in transit in Layer 2. How can I build a state channel that is secure in that situation?

I think this ADR should have detailed analysis and proposed solutions for a few such problems. Basically any layer 2 solution will face these issue.

Copy link
Contributor

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can cause lots of unintended and undiscovered impacts.

Please take a look at https://github.com/CosmWasm/cosmwasm-plus/tree/master/contracts/cw1-whitelist which is a super minimal contract. If you give tokens to this, it provides one clear address and you can swap under that. However, the account address never has a pubkey associated with it. And it actually provides more options that what you describe here and is easily extended in eg cw1-subkeys

I think you could do something very similar in a native module, making a lot of "sub-module addresses", which may have a rotating pubkey (or keys) behind them. But since the address never has a pubkey directly when querying x/auth, then you never create confusion. Either an account is controlled by an off-chain pubkey or controlled by on-chain logic.


## Context

Currently, in the Cosmos SDK, the address of an auth `BaseAccount` is based on the hash of the public key. Once an account is created, the public key for the account is set in stone, and cannot be changed. This can be a problem for users, as key rotation is a useful security practice, but is not possible currently. Furthermore, as multisigs are a type of pubkey, once a multisig for an account is set, it can not be updated. This is problematic, as multisigs are often used by organizations or companies, who may need to change their set of multisig signers for internal reasons.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flexible on chain multisigs are done in CosmWasm and coming soon in SDK. I don't see this point as a reason to add the pubkey rotation which has many negative impacts on client code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tend to agree here @ethanfrey


### Positive

* Will allow users and validator operators to employ better operational security practices with key rotation.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

validator operators typically use multisigs. They could just move to new on-chain multisig as soon as they are available.

Breaks the current assumed relationship between address and pubkeys as H(pubkey) = address. This has a couple of consequences.

* We cannot prune accounts with 0 balance that have had their pubkey changed (we currently do not currently do this anyways, but the reason we have account numbers is presumably for this purpose).
* This makes wallets that support this feature more complicated. For example, if an address on chain was updated, the corresponding key in the CLI wallet also needs to be updated.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is far more than a UI issue. If you have any intelligent client node, like a long-living program, especially if it does any caching or rollup. This will make life harder, possibly impossible to resolve.

@jackzampolin
Copy link
Member

I tend to agree with @ethanfrey here, this is pretty huge change and it seems like the usecases we are envisioning are taken care of by the group module. Breaking a bunch of client side assumptions right as we are getting to stable client side libs seems bad.


## Context

Currently, in the Cosmos SDK, the address of an auth `BaseAccount` is based on the hash of the public key. Once an account is created, the public key for the account is set in stone, and cannot be changed. This can be a problem for users, as key rotation is a useful security practice, but is not possible currently. Furthermore, as multisigs are a type of pubkey, once a multisig for an account is set, it can not be updated. This is problematic, as multisigs are often used by organizations or companies, who may need to change their set of multisig signers for internal reasons.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tend to agree here @ethanfrey

@robert-zaremba
Copy link
Collaborator

During the call I mentioned that I made a comment with few ideas how to support verifiable off-chain signatures:

  1. Attach blockchain transaction ID to the MsgSignData (as a required field). Verifier checks with the blockchain that the transaction exists and is signed by the same key.
  2. Similarly to the above one: instead of attaching a transaction ID, we can attach a merkle proof + block height, proving that at that time, an account has assigned correct public key.
  3. Update this ADR (ADR-34) to store the past public keys. If someone will rekey, we will have a stack of the previous keys. Dapp can query that stack to search for a public key.

I think the first one is the easiest one. Note it's also sound, because whenever a user will rekey, he will need to do a transaction with his old key. So for every users' public key, if the key was associated with a blockchain account, there will be always an associated blockchain transaction.


## Decision

We propose the addition of a new feature to `x/auth` that allows accounts to update the public key associated with their account, while keeping the address the same. This feature can be enabled using an `EnableChangePubKey` param.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the call it seemed like people thought this EnableChangePubKey param was superfluous... but I'm also not sure it hurts to have it.

@sunnya97
Copy link
Member Author

@ethanfrey

There is a nice separation in Ethereum from "external accounts" - which are tied 1:1 with a pubkey for authorization and "contract accounts' - which are controlled by logic.

There is a push in the Ethereum to actually remove EOAs and have only contract accounts (see EIP 2938 - Account Abstraction)

Client tooling. One big hope was coordination off-chain - things like authenticating as in ADR 036, but also wallet connect, creating multisigs, etc.

By designing client tooling to be focused on working with accounts that cannot change pubkeys, this will likely leave accounts that can (on-chain multisigs, smart contract wallets, etc) out in the dust. This is not desirable (this happens already on some off-chain coordination tools like Snapshot). Instead, we should make it clear that it is expected that in the general-case, the authentication logic of accounts is mutable and composed of logic. And for applications where this does not work, these applications should explicitly special case fixed pubkeys in their modules. See next section:

especially on the off-chain / wallet / layer 2 side. I do not see any clear analysis of how one could build off-chain infrastructure with this key rotations. ... e.g. if I open a state channel with an account, I would only open it with a pubkey tied to an EOA.

For things that need long term stable keys, their modules should explicitly lock users into such keys. For example, when designing a payment channel module, the payment channel shouldn't be based on the public key of the accounts, but rather a new public key should be stored in the Payment channel struct. This could be the same public key as the account's public key, but it would not change if the account's public key changes.

@sunnya97
Copy link
Member Author

I've added the pruning details to the ADR requested by @robert-zaremba and @alexanderbez

I've also added to the ADR the addition of option 3 from @robert-zaremba's list of proposals on how to support verifiable off-chain signatures, as its the one I find the simplest for clients to implement.

Update this ADR (ADR-34) to store the past public keys. If someone will rekey, we will have a stack of the previous keys. Dapp can query that stack to search for a public key.

@robert-zaremba
Copy link
Collaborator

This could be the same public key as the account's public key, but it would not change if the account's public key changes.

I would coin a term: Special Purpose Account :)

@sunnya97
Copy link
Member Author

sunnya97 commented Nov 23, 2020

This could be the same public key as the account's public key, but it would not change if the account's public key changes.

I would coin a term: Special Purpose Account :)

I was thinking it wouldn't even be an Account. The struct for a Payment channel would look like:

struct PayChan {
    user1Addr        sdk.Address
    user1PubKey   crypto.PubKey

    user2Addr       sdk.Address
    user2Pubkey   crypto.PubKey

    ...
    challengePeriod
    amount
    lastState
    ...
}

It uses user1Addr and user2Addr as the source of funds and payout addresses, but uses user1PubKey and user2Pubkey as the pubkeys involved with the payment channel update signing

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sunnya97 let's remove the EnableChangePubKey section. Also, what arguments do you have for this feature that CosmWasm would not address.

I think we can keep and merge this ADR as proposed but I think we need more discussion around it, especially if CosmWasm already buys us this functionality.


### Positive

* Will allow users and validator operators to employ better operational security practices with key rotation.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could instead devise a way to provide a means of one-time "migrating".

@sunnya97
Copy link
Member Author

sunnya97 commented Nov 24, 2020

@alexanderbez

While you could use CosmWasm to do rotating keys, there are many SDK-based chains that will probably not want to integrate CosmWasm functionality, due to variety of reasons. And it will likely be a much while longer before CosmWasm is included on major chains like the Cosmos Hub. This is a much simpler feature that can be integrated very quickly

Copy link
Collaborator

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating the description and discussing the outcomes.

@sunnya97 sunnya97 merged commit a648325 into cosmos:master Nov 27, 2020
@alexanderbez
Copy link
Contributor

alexanderbez commented Nov 27, 2020

Why just create your own module @sunnya97 and publish it to Atlas? Developers/chains that need such functionality w/o CosmWasm can simple use your module.

@sunnya97
Copy link
Member Author

sunnya97 commented Dec 1, 2020

@alexanderbez well that's why I proposed having a param that can toggle it on and off, to effectively do that without needing to maintain two different modules

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:x/auth T: ADR An issue or PR relating to an architectural decision record
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants